Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-38670][SS] Add offset commit time to streaming query listener #35985

Closed
wants to merge 2 commits into from
Closed

[SPARK-38670][SS] Add offset commit time to streaming query listener #35985

wants to merge 2 commits into from

Conversation

jerrypeng
Copy link
Contributor

@jerrypeng jerrypeng commented Mar 28, 2022 •
edited
Loading

What changes were proposed in this pull request?

Add a metric called "commitOffsets" to the StreamingQueryListener

Why are the changes needed?

A good portion of the batch duration is committing offsets at the end of the micro-batch. The timing for this operation is missing from the durationMs metrics. Lets add this metric to have a more complete picture of where the time is going during the processing of a micro-batch

Does this PR introduce any user-facing change?

Yes, an event returned from the StreamingQueryListener will now contain an additional metric:

  "durationMs" : {
    "addBatch" : 39,
    "getBatch" : 0,
    "latestOffset" : 0,
    "queryPlanning" : 9,
    "triggerExecution" : 228,
    "walCommit" : 82,
    "commitOffsets": 81 // <---- added this metric
  },

How was this patch tested?

add a test

HeartSaVioR
HeartSaVioR reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add [SS] after [SPARK-38670].

We have a stack graph with regarding to the duration in the micro batch in SS UI - I'm not sure the change is reflected on the graph.

Could you please run a e2e manually and make sure the change is also reflected in the SS UI page as well? Please attach the screenshot and the step you've performed on manual test in the section of "How was this patch tested?".

This PR does introduce the user-facing change since we add the new field for durationMs in streaming listener. Could you please update the section "Does this PR introduce any user-facing change?" to describe the change?

Thanks in advance!

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQuerySuite.scala
@@ -316,6 +316,7 @@ class StreamingQuerySuite extends StreamTest with BeforeAndAfter with Logging wi
assert(query.recentProgress.last.eq(query.lastProgress))

val progress = query.lastProgress

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unnecessary change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will revert

@jerrypeng jerrypeng changed the title [SPARK-38670] Add offset commit time to streaming query listener [SPARK-38670][SS] Add offset commit time to streaming query listener Mar 28, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@jerrypeng
Copy link
Contributor Author

Screen Shot 2022-03-28 at 3 58 17 PM

An image from the UI, the new metric shows up correctly.

@jerrypeng
Copy link
Contributor Author

@HeartSaVioR thanks for the review. Can you please take another look?

HeartSaVioR
HeartSaVioR approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@HeartSaVioR
Copy link
Contributor

Thanks! Merging to master.

@HeartSaVioR
Copy link
Contributor

Thanks @jerrypeng for your contribution! I merged this to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

Assignees
No one assigned
Labels
SQL STRUCTURED STREAMING
Projects
None yet
Milestone
No milestone
3 participants
@jerrypeng @AmplabJenkins @HeartSaVioR